On Wed, Mar 02, 2016 at 10:38:40PM -0800, Josh Allmann wrote:

Not really intended for merging, just a preview of work so far -- I'm still learning ppx too. Feedback welcome.

Looking at it (but I only know a little more ppx than last week, mostly from
figuring out how your code works, so I can't promise I will be of much help).

Firstly, this adds an extension on Ppx_core for AST folding. The setup for
that is quite complex (thanks @Drup for help with this and all my other ppx
questions...). The biggest issue is that I think using Ppx_core introduces
a transitive dependency on OCaml >= 4.02.3, which is probably undesirable
for sqlexpr. I can either try reworking the opam/oasis files to compile
ppx only conditionally (hopefully that's possible?), or implement a fold
using OCaml's Ast_iterator.

I had already assumed it would become difficult to support ppx without
breaking the build on older versions, so I've already made a 0.6 branch meant
to be buildable with 4.01 (so master with ppx support would be released as 0.7
or, why not, 1.0). Frankly, this represents less work than fighting oasis/opam
(I don't foresee more than a couple releases in the 0.6 branch, with some
bugfixes from master).

I think it's acceptable to drop ppx support for 4.02.[12]: it's never been
released, having been been merged for barely a week, so we're not breaking
anybody's code yet.

So unless somebody else comes up with a good reason not to, AFAIAC you can
feel free to introduce a >= 4.02.3 dependency if that means less work for you.

The ppx tests for sqlexpr all pass, but this doesn't yet hoist everything
that pa_estring does: handling for Pstr_eval and Pstr_class are
missing. The latter simply hasn't been done yet, and the former [1] leads to
some very strange interactions with Lwt bindings [2]. (Probably a bug on my
end, but I haven't figured it out yet...)

Is Pstr_eval of any practical significance? If I understand it correctly, it's
for top-level expressions being evaluated right away when the module is
initialized(?).

As for Pstr_class, you could argue that you're not very concerned about
speed once you're dealing with objects (and failing to hoist sqlexpr
statements/expressions should have a very limited impact).

They're minor use cases for a minor optimization, not worth the trouble if
they prove difficult.

Finally, a thought I had while implementing this: it might be more idiomatic
to use an annotation alongside the [%sql ...] declaration, eg [@cached]
or similar, rather than defining [%sqlc ...] as a separate extension.
Since the work for [%sqlc ...] has been done already, probably doesn't
matter too much.

Cannot really say a lot about idiomatic ppx uses, as a non-ppx-user-yet...
If you ask me, [%sqlc ...] is OK and already done :)

Mauricio Fernández